Just reading through the code and the detailed explanation you've outlined in the PR, the one thing I wanted to check related to this:

The new proposer identifies the Round Change (RC) message with the highest prepared round and bases a new proposal on the attached prepared message for that highest round. The proposal’s block header reflects the current round number, but the prepared message still references the prior round.

Your fix sounds like it address this by effectively ignoring the round number. I'm wondering if there's an alternative fix which is to make sure the round number in the proposal message is correct for the new round?

@jframe I wonder if you've got time to take a look at this PR that Bhanu has raised to see what you think?

Is it easy to write a test for this? Would be nice to add a regression test if possible. Will add an approval once I've ready through the forCommittedSeal vs forOnchainBlcok code a bit more thoroughly.

Yes I was thinking about the test , will check if it's relatively easy to add one.

Just reading through the code and the detailed explanation you've outlined in the PR, the one thing I wanted to check related to this:

The new proposer identifies the Round Change (RC) message with the highest prepared round and bases a new proposal on the attached prepared message for that highest round. The proposal’s block header reflects the current round number, but the prepared message still references the prior round.

Your fix sounds like it address this by effectively ignoring the round number. I'm wondering if there's an alternative fix which is to make sure the round number in the proposal message is correct for the new round?

@jframe I wonder if you've got time to take a look at this PR that Bhanu has raised to see what you think?

That's true, I got it incorrect about inclusion of round number . Currently (before this PR) prepared block and proposal both don't include round number in hash calculation . So , as far as validation goes , it should only be checked if prepared and proposed blocks hashes are equal (without any round number replacement) , which happens to be the case based on my experimentation. Prepared round and proposal round info are in metadata .

Made changes to fix the other way as proposed above - adding round numbers to new block proposals and updating the round number proposals based on prepared blocks to current round. updates are made in the block header .

Tested and verified the fix manually , will continue looking into adding a testcase

Figured that there's already a testcase which could have caught this error -

It throws the validation error in logs

2024-11-14 16:02:16.999-05:00 | Test worker | INFO  | RoundChangeManager | Address: 0xb02c0093e94b417620f24d8857d6a1dd3d230272  Round: 4
2024-11-14 16:02:17.013-05:00 | Test worker | INFO  | RoundTimer | BFT round 3 expired. Moved to round 4 which will expire in 192 seconds
2024-11-14 16:02:17.029-05:00 | Test worker | INFO  | ProposalValidator | Invalid Proposal Payload: Latest Prepared Metadata blockhash does not align with proposed block
Task :consensus:qbft:integrationTest 

but doesn’t fail due to another bug in the code - while proposer handles their own proposal, it doesn’t check if the proposal is valid before sending prepare. Just fixing that made the above test fail. Then included the change to add round number in block header extradata, corresponding test case changes and verified all the testcases in consensus/qbft/src/integration-test/java/org/hyperledger/besu/consensus/qbft/test pass.

This feels like a better fix to me @pullurib, thanks for tweaking it and for the extra tests.

Since this fixes a number of issues, think it's worth adding a changelog entry @pullurib

Since this fixes a number of issues, think it's worth adding a changelog entry @pullurib

Done, thanks